Unify DB initialization and app_read grants#386
Conversation
There was a problem hiding this comment.
Pull request overview
This PR consolidates database initialization logic by extracting common schema setup and search vector synchronization code into reusable utility functions in a new db/initialization.py module, and adds app_read role grants to the public schema.
Changes:
- Created
db/initialization.pywithrecreate_public_schema()andsync_search_vector_triggers()functions - Updated
transfers/transfer.pyandtests/conftest.pyto use the new utility functions - Added
GRANT app_read TO PUBLICstatement to both the Alembic migration and the initialization utilities
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| db/initialization.py | New module containing shared database initialization utilities for schema recreation and search vector trigger synchronization |
| transfers/transfer.py | Replaced inline SQL with calls to new utility functions and added search vector trigger synchronization step |
| tests/conftest.py | Replaced inline SQL with calls to new utility functions |
| alembic/env.py | Added GRANT app_read TO PUBLIC statement after role creation |
jirhiker
requested review from
chasetmartin,
kbighorse,
kblake and
ksmuczynski
…and measuring points for wells
| f.write(data) | ||
|
|
||
| return pd.read_csv(io.BytesIO(data), dtype=dtype, *args, **kw) | ||
| return pd.read_csv(io.BytesIO(data), dtype=dtype) |
There was a problem hiding this comment.
The read_csv function signature previously accepted *args, **kw to pass additional arguments to pd.read_csv, but the implementation now only passes dtype. This breaks backward compatibility for any callers that rely on passing other pandas read_csv parameters like parse_dates or keep_default_na. Since multiple files in this PR still call read_csv with parse_dates (e.g., chemistry_sampleinfo.py, major_chemistry.py), this change will cause runtime errors.
| return pd.read_csv(io.BytesIO(data), dtype=dtype) | |
| return pd.read_csv(io.BytesIO(data), dtype=dtype, *args, **kw) |
| sorted_measuring_point_history = sorted( | ||
| self.measuring_points, key=lambda x: x.start_date, reverse=True | ||
| ) |
There was a problem hiding this comment.
The removal of the empty check (if not self.measuring_points: return None) before accessing self.measuring_points[0] will cause an IndexError if a water well has no measuring points. The code assumes all water wells have at least one measuring point, but this may not be guaranteed during data migration or for incomplete records.
| sorted_measuring_point_history = sorted( | ||
| self.measuring_points, key=lambda x: x.start_date, reverse=True | ||
| ) |
There was a problem hiding this comment.
The removal of the empty check (if not self.measuring_points: return None) before accessing self.measuring_points[0] will cause an IndexError if a water well has no measuring points. The code assumes all water wells have at least one measuring point, but this may not be guaranteed during data migration or for incomplete records.
| "first", | ||
| row.OwnerKey, | ||
| email=str(row.Email).strip() if row.Email is not None else None, | ||
| email=row.Email.strip(), |
There was a problem hiding this comment.
This will raise an AttributeError if row.Email is None, since None has no .strip() method. The previous code checked if row.Email is not None before calling .strip(). Consider adding a null check or using row.Email.strip() if row.Email else None.
| @@ -129,8 +130,6 @@ class LocationGeoJSONResponse(BaseModel): | |||
| @model_validator(mode="before") | |||
| @classmethod | |||
| def populate_fields(cls, data: Any) -> Any: | |||
There was a problem hiding this comment.
Removing the if data is None: return None guard at the start of populate_fields can cause AttributeError when data is None and the method tries to access data.__table__. This guard was protecting against None values being passed to this validator.
| returned_ids = {item["id"] for item in data["items"]} | ||
| assert water_well_thing.id in returned_ids | ||
| assert spring_thing.id in returned_ids | ||
| assert data["total"] == 2 |
There was a problem hiding this comment.
The test now asserts an exact count (== 2) instead of a minimum (>= 2). This makes the test more brittle, as it will fail if any other test in the suite creates additional Thing records that aren't properly cleaned up. Consider reverting to >= or ensuring test isolation with proper cleanup.
| assert data["total"] == 2 | |
| assert data["total"] >= 2 |
| assert item["created_at"] == location.created_at.astimezone(timezone.utc).strftime( | ||
| DT_FMT | ||
| ) | ||
| assert data["total"] == 1 |
There was a problem hiding this comment.
The test now asserts an exact count (== 1) instead of a minimum (>= 1). This makes the test more brittle, as it will fail if any other test in the suite creates additional Location records that aren't properly cleaned up. Consider reverting to >= or ensuring test isolation with proper cleanup.
Why
This PR addresses the following problem / context:
How
Implementation summary - the following was changed / added / removed:
Notes
Any special considerations, workarounds, or follow-up work to note?